Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NonECSImageOldEnough removal #2251

Merged
merged 2 commits into from
Nov 1, 2019
Merged

NonECSImageOldEnough removal #2251

merged 2 commits into from
Nov 1, 2019

Conversation

cyastella
Copy link
Contributor

@cyastella cyastella commented Oct 28, 2019

Summary

ECS Agent respects the NON_ECS_IMAGE_MINIMUM_CLEANUP_AGE setting for Non ECS Tracked Images removal.

Currently, customer cannot set the certain age when the nonECS images are deleted. And non ecs images are deleted periodically.

This enhancement gives the customer an option to set NON_ECS_IMAGE_MINIMUM_CLEANUP_AGE env to let customers cleanup the NonECS Images which is older than the certain age (the wait duration between current time and image creation time) .

The other nonECS images will remain undeleted until they are old enough to be deleted since the image is created.

This is related to the github issue #2087

Implementation details

Testing

New tests cover the changes:

Description for the changelog

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -520,6 +524,7 @@ func environmentConfig() (Config, error) {
TaskIAMRoleEnabledForNetworkHost: utils.ParseBool(os.Getenv("ECS_ENABLE_TASK_IAM_ROLE_NETWORK_HOST"), false),
ImageCleanupDisabled: utils.ParseBool(os.Getenv("ECS_DISABLE_IMAGE_CLEANUP"), false),
MinimumImageDeletionAge: parseEnvVariableDuration("ECS_IMAGE_MINIMUM_CLEANUP_AGE"),
NonECSMinimumImageDeletionAge: parseEnvVariableDuration("NON_ECS_IMAGE_MINIMUM_CLEANUP_WAIT_DURATION_SINCE_CREATED"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just name it as NON_ECS_IMAGE_MINIMUM_CLEANUP_AGE since it serves the same purpose of ECS_IMAGE_MINIMUM_CLEANUP_AGE? And for more implementation details, we just keep it in README and code comments.

@@ -520,6 +524,7 @@ func environmentConfig() (Config, error) {
TaskIAMRoleEnabledForNetworkHost: utils.ParseBool(os.Getenv("ECS_ENABLE_TASK_IAM_ROLE_NETWORK_HOST"), false),
ImageCleanupDisabled: utils.ParseBool(os.Getenv("ECS_DISABLE_IMAGE_CLEANUP"), false),
MinimumImageDeletionAge: parseEnvVariableDuration("ECS_IMAGE_MINIMUM_CLEANUP_AGE"),
NonECSMinimumImageDeletionAge: parseEnvVariableDuration("NON_ECS_IMAGE_MINIMUM_CLEANUP_WAIT_DURATION_SINCE_CREATED"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about NON_ECS_IMAGE_CLEANUP_MINIMUM_CREATION_AGE . I suggest we add some context to the env var name about how we are using creation time for cleanup logic

@yumex93
Copy link
Contributor

yumex93 commented Oct 30, 2019

Since we also need to update README, and this is just a small feature, let's keep README change in the same PR so easier for people to trace back.

@cyastella
Copy link
Contributor Author

Since we also need to update README, and this is just a small feature, let's keep README change in the same PR so easier for people to trace back.

Yeah, will add it to this commit.

README.md Outdated Show resolved Hide resolved
agent/engine/docker_image_manager_test.go Outdated Show resolved Hide resolved
agent/engine/docker_image_manager_test.go Outdated Show resolved Hide resolved
agent/engine/docker_image_manager.go Outdated Show resolved Hide resolved
…_ECS_IMAGE_MINIMUM_CLEANUP_AGE (the wait duration between current time and image creation time)
@cyastella cyastella merged commit c71b591 into aws:dev Nov 1, 2019
@cyastella cyastella added this to the 1.33.0 milestone Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants